Skip to content

Conversation

@ValentinVERGEZ
Copy link
Contributor

@ValentinVERGEZ ValentinVERGEZ commented Jul 13, 2016

Ne pas intégrer.


This change is Reviewable

@vcoelen
Copy link

vcoelen commented Jul 14, 2016

Attention, les fichiers avec une extension terminant par un "" (ex CMakeLists.txt ...) sont des fichiers de backup créés par certains éditeurs (ex gedit, ...), il ne faut pas les intégrer à git.
Pour le code, je n'ai rien à dire pour le moment.


Review status: 0 of 10 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

…constructeur et modification des données membres, finalisation des transmitions du beacon pour la node de test et le superviseur et insertion du tableau vectoriel
@ValentinVERGEZ
Copy link
Contributor Author

Plusieurs remarques assez générales, appliquées à ton code du nœud de test.
J'ai pas regardé au superviser.
Si t'as des question n'hésite pas.


Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions.


supervizer/src/node_test.cpp, line 18 [r2] (raw file):

void transmission_stockage(void)
{

Même remarque qu'en dessous (désolé, je fais la revue à l'envers :trollface: )


supervizer/src/node_test.cpp, line 27 [r2] (raw file):

{
  supervizer::beacon msg;
  ros::Publisher chatter_pub = g_nhNode->advertise<std_msgs::Time>("beacon", 1000);

Bon, ce coup ci ça va fonctionner (je fais référence à un commentaire plus loin au sujet su subscriber), mais c'est pas top.
Tu déclares ton publisher dans la fonction, donc son scope est limité, donc à la fin de la fonction il sera détruit.
Alors ok ça fonctionnera, mais ça oblige ton nœud à contacter roscore toutes les 100ms (loop_rate à 10Hz) pour indiquer que tu comptes publier sur "beacon", ce qui est inutilement lourd.
Ce que je te conseille, déclare ton publisher en global ou dans le main. Si tu le déclares dans le main, tu pourras le passer en référence à ta fonction.

Tant qu'on y est dans l'économie, ton supervizer::beacon peut être déclaré en static pour qu'il ne soit construit qu'une fois. Mais bon ça osef ...


supervizer/src/node_test.cpp, line 51 [r2] (raw file):

  void chatterCallback(const std_msgs::Time &msg)
{
  //TODO Adapter aux echanges complets et penser a enregistrer les données

Oui oui. Bien plus tard, quand le supervizer fonctionnera avec stockage de données, faudra essayer d'avoir des données qui évoluent en fonction du temps.
Pour l'instant tu envoies le temps présent, tu pourras par exemple envoyer le carré tu temps écoulé depuis le début du nœud.
Et pour prouver le concept, il faudra que le nœud, au démarrage, cherche à obtenir le temps précédemment envoyé (cas d'un respawn à cause d'un crash).
Mais on n'y est pas encore. C'est franchement déjà pas mal ce que tu fais.

Faudra aussi penser à renommer ta callback 😛


supervizer/src/node_test.cpp, line 58 [r2] (raw file):

{
  ros::Subscriber sub = pN->subscribe(g_sNomIn, 10, chatterCallback);
  ros::spin();

Attention, ici tu vas te créer des problèmes.

Ton subsciber a un scope limité à la fonction reception. Ça signifie que lorsque ton fil d’exécution quittera la fonction, le subscriber sera détruit et la callback chatterCallback ne pourra plus être appelée.
Comme ros::spin() est bloquant, la fonction ne rendra pas la main, le subscriber ne sera pas détruit et donc tu recevras bien des trucs sur chatterCallback.
En revanche tous le code que tu as après ton appel à reception ne sera jamais appelé.

Ce que je te conseille ici, c'est d'avoir soit :

  • ton subscriber déclaré dans le main, que tu passerais en référence à la fonction reception
  • un subscriber déclaré en global

Et supprimes le ros:spin d'ici.
Laisse la boucle de ton main avec son spinOnce faire le boulot.

Dernière remarque sur la fonction.
Tel que ça se présente dans cette révision, tu n'as pas réellement besoin d'une fonction pour souscrire au topic. Fait le directement dans le main.


supervizer/src/node_test.cpp, line 67 [r2] (raw file):

    g_sNomOut = "Out_nt";
    g_sNomIn ="In_nt";
    ros::init(argc, argv, g_sNomNode_);

g_sNoneNode (underscore en trop)


supervizer/src/node_test.cpp, line 78 [r2] (raw file):

    while (ros::ok())
      {
        transmission_beacon

transmission_beacon()

Oui je sais, je sais, tu m'as dit que t'avais pas encore compilé 😄


supervizer/src/node_test.cpp, line 85 [r2] (raw file):

  return 0;
  }

Manque un retour chariot à la fin du fichier. Son absence ne tuera personne, mais c'est mieux avec. (En plus c'est demandé par le standard C et C++)


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants